Improve lane changing vis-a-vis uber turns. #412
Conversation
1. Allow lane changing in an uber turn. Because of the way uber turns work, we lock in and commit to all the lane changes just before entering the uber turn. 2. Avoid overzealous lane changing by combining number-of-lanes-crossed and numer-of-vehicles-in-lane into a single cost, rather than always preferring the least number-of-vehicles-in-lane. 3. Don't lane-change unless the candidate lane's cost is strictly better than the current lane cost.
| @@ -16,7 +16,7 @@ impl Logger { | |||
| pub fn setup() { | |||
| #[cfg(target_arch = "wasm32")] | |||
| { | |||
| console_log::init_with_level(log::Level::Debug).unwrap(); | |||
| console_log::init_with_level(log::Level::Info).unwrap(); | |||
There was a problem hiding this comment.
I often spend some time engineering nice logging, but find it's too spammy for general purpose. I'd like to leave the spammy logging at debug level and bump up our default log level.
Relatedly, how would you feel about incorporating something like env_logger so the user can adjust their log level without recompiling?
There was a problem hiding this comment.
Info by default SGTM, and so does env_logger. I've meant to consolidate Timer with logging for a while: #262
|
|
||
| // When replacing a turn, also update any references to it in uber_turns | ||
| if let PathStep::Turn(old_turn) = self.steps[idx] { | ||
| for uts in &mut self.uber_turns { |
There was a problem hiding this comment.
This is kind of ham-fisted. Since, at the call-site we know something about the offset of our turn it's possible we could do something smarter, but it got a little complicated. I profiled this and it didn't contribute any meaningful runtime, so I decided to keep it like this.
There was a problem hiding this comment.
Simpler code SGTM since performance is fine
| }) | ||
| .min_by_key(|(cost, _, _, _)| *cost) | ||
| .unwrap(); | ||
| // TODO Only switch if the target queue is some amount better; don't oscillate |
There was a problem hiding this comment.
I have noticed a pattern where it seems like in general long trips are improved while short trips suffer. I'm not really sure why. One guess would be that previously longer trips were more likely to be clogged up in the major thoroughfares, leaving the back roads relatively empty.
I wouldn't read too much into this, unless you dig into individual cases. Parking makes things super sensitive; slight changes in one part of the map could mean somebody else snags a spot first, and the other person has to go much farther to park. Ideally we'd show this difference more clearly when comparing to prebaked results. If you ever need to get rid of this difference, running both baseline and experiment with --infinite_parking does the trick.
One interesting thing - this change doesn't really seem to help anything until there's a bit of traffic. e.g. in the Krakow case there's basically 0 extra trips from 12:00am-7:00am, but then by 7:30 when traffic picks up we suddenly start to rack up extra trips.
This seems somewhat intuitive. If there's not many people around, discretionary LCing won't happen as much, and it won't matter even when it does.
From looking at that, it seems like things are worse than before. However I'm wondering if this is due to their being more completed trips in the after scenario than in the before scenario. Maybe these charts weren't meant to compare to partial/incomplete prebakes.
This is a bit confusing indeed. This dashboard uses analytics.both_finished_trips, so it should skip over incomplete trips in before or after. I think it may be more clear to include (incomplete, complete) or (complete, incomplete) cases in the scores shown here, treating the duration of incomplete trips as now - start. But that subtlety needs to be communicated somehow.
As a side note, would you be open to including the partial day south Seattle and Krakow into the default prebake? It might be nice to help avoid regressing there.
This would be great!
LGTM, thanks for working through this beast of a problem! I have the re-prebaked data ready to upload, will do it right after merging.
Looking at Krakow with this fix, I believe the next problem is actually over-eager uber-turn definitions. At i65 by about 7am, the roundabout is mostly full, and c184467 refuses to go:
If you check the uber-turns here, you see the problem:
Because the destination isn't clear, the car doesn't even start the UT. But if they did, things might unroll and flow again. I thought the new blocked-by graph (ctrl+D, b) would show the issue, but it seems like c184467 isn't registering a depenency on the cars in the northern chunk. That's likely because they're not in the next lane; they're a few away, at the end of the UT. So possibly the next step is to make blocked_by understand these far-away dependencies, so the existing break-the-cycle works. Or maybe we should adjust the definition of UTs and not create them here at all, since there's room for about 2 cars on the currently empty north/south roads?
| @@ -16,7 +16,7 @@ impl Logger { | |||
| pub fn setup() { | |||
| #[cfg(target_arch = "wasm32")] | |||
| { | |||
| console_log::init_with_level(log::Level::Debug).unwrap(); | |||
| console_log::init_with_level(log::Level::Info).unwrap(); | |||
There was a problem hiding this comment.
Info by default SGTM, and so does env_logger. I've meant to consolidate Timer with logging for a while: #262
|
|
||
| // When replacing a turn, also update any references to it in uber_turns | ||
| if let PathStep::Turn(old_turn) = self.steps[idx] { | ||
| for uts in &mut self.uber_turns { |
There was a problem hiding this comment.
Simpler code SGTM since performance is fine
Effectively three behavioral changes:
Allow lane changing in an uber turn. Because of the way uber turns work, we lock in and commit to all the lane changes just before entering the uber turn.
Avoid overzealous lane changing by combining number-of-lanes-crossed and numer-of-vehicles-in-lane into a single cost, rather than always preferring the least number-of-vehicles-in-lane.
Don't lane-change unless the candidate lane's cost is strictly better than the current lane cost.
Results (vs. prebake run on b5ea263):
Montlake and Lakeslice are affected, but seemingly not by much. In both case the "saved time" slightly outweighs the "lost time".
I have noticed a pattern where it seems like in general long trips are improved while short trips suffer. I'm not really sure why. One guess would be that previously longer trips were more likely to be clogged up in the major thoroughfares, leaving the back roads relatively empty.
montlake
lakeslice
south seattle (until 8am)
Things are pretty deadlocked by 8am, but on average, things are a little bit better for this map.
before: 34,366 agents left by end of day
after: 33,923 agents left by end of day
difference: 443 more trips completed
Krakow (until 9am)
before: 14,465 agents left by end of day
after: 13,686 agents left by end of day
difference: 1,779 more trips completed
Krakow's improvement is a bit more pronounced. My hypothesis is that this change mostly helps with complex compound intersections, of which Krakow has no shortage.
Here's the GIF that started this whole pursuit:
before:
and here's the behavior after:
Note people are much more likely to stay in their lanes, and all the lanes get used.
One interesting thing - this change doesn't really seem to help anything until there's a bit of traffic. e.g. in the Krakow case there's basically 0 extra trips from 12:00am-7:00am, but then by 7:30 when traffic picks up we suddenly start to rack up extra trips.
One thing that surprised me here:
...so by 8am we have 1k+ bonus trips.
But the stats show slower trips.
From looking at that, it seems like things are worse than before. However I'm wondering if this is due to their being more completed trips in the after scenario than in the before scenario. Maybe these charts weren't meant to compare to partial/incomplete prebakes.
As a side note, would you be open to including the partial day south Seattle and Krakow into the default prebake? It might be nice to help avoid regressing there.